Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

crypto/keyring: change addrKey to store chain-agnostic addresses #5858

Merged
merged 4 commits into from
Mar 30, 2020
Merged

crypto/keyring: change addrKey to store chain-agnostic addresses #5858

merged 4 commits into from
Mar 30, 2020

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Mar 24, 2020 •
edited by alexanderbez
Loading

Keyrings store keys by name and hexbytes representation
of address. This turns keyring internal storage more
chain-agnostic and types.Config independent.
Obsolete Keybase internal state representation is not affected.

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@lgtm-com
Copy link

lgtm-com bot commented Mar 24, 2020

This pull request introduces 1 alert when merging c8163ba into a1ac04c - view on LGTM.com

new alerts:

  • 1 for Unreachable statement

@codecov
Copy link

codecov bot commented Mar 24, 2020 •
edited
Loading

Codecov Report

Merging #5858 into master will increase coverage by 0.01%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master    #5858      +/-   ##
==========================================
+ Coverage   56.75%   56.77%   +0.01%     
==========================================
  Files         344      344              
  Lines       20294    20307      +13     
==========================================
+ Hits        11518    11529      +11     
- Misses       7895     7896       +1     
- Partials      881      882       +1

alexanderbez
alexanderbez reviewed Mar 24, 2020
View reviewed changes
client/keys/show.go Outdated Show resolved Hide resolved
alexanderbez
alexanderbez reviewed Mar 24, 2020
View reviewed changes
client/keys/show.go Outdated Show resolved Hide resolved
@alessio alessio force-pushed the alessio/store-keys-by-hexbytes-repr branch 2 times, most recently from 266ba0e to 6819337 Compare March 26, 2020 16:36
@alessio alessio changed the title Store keys by name and address's hexbytes repr crypto/keyring: change addrKey to store chain-agnostic addresses Mar 26, 2020
@alessio alessio added C:Keys Keybase, KMS and HSMs R4R labels Mar 26, 2020
@alessio alessio marked this pull request as ready for review March 26, 2020 16:37
alexanderbez
alexanderbez reviewed Mar 26, 2020
View reviewed changes
crypto/keyring/keyring.go Outdated
@@ -581,3 +582,7 @@ func newRealPrompt(dir string, buf io.Reader) func(string) (string, error) {
}
}
}

func addrKey(address types.AccAddress) []byte {
return []byte(fmt.Sprintf("%s.%s", hex.EncodeToString(address.Bytes()), addressSuffix))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why encode to hex when we can just use the String interpretation?

Copy link
Contributor Author

@alessio alessio Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because accounts types String methods depend on the notorious Config singleton

Copy link
Contributor Author

@alessio alessio Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't change it for the old Keybase implementations to avoid breaking gaiacli versions used for Cosmos Hub

@alessio alessio force-pushed the alessio/store-keys-by-hexbytes-repr branch from 145e84f to 59d58f8 Compare March 26, 2020 20:11
Alessio Treglia added 2 commits March 28, 2020 18:25
crypto/keyring: change addrKey to store chain-agnostic addresses
5738f71 
Keyrings store keys by name and hexbytes representation
of address. This turns keyring internal storage more
chain-agnostic and types.Config independent.
Obsolete Keybase internal state representation is not affected.
crypto/keyring: change addrKey to store chain-agnostic addresses
7a74fe4 
Keyrings store keys by name and hexbytes representation
of address. This turns keyring internal storage more
chain-agnostic and types.Config independent.
Obsolete Keybase internal state representation is not affected.
@alessio alessio force-pushed the alessio/store-keys-by-hexbytes-repr branch from ae0c933 to 7a74fe4 Compare March 28, 2020 17:27
@alessio
Copy link
Contributor Author

alessio commented Mar 29, 2020

@alexanderbez comments have been incorporated, please review again when you spare a minute

alexanderbez
alexanderbez approved these changes Mar 30, 2020
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@alessio alessio merged commit db76afe into master Mar 30, 2020
@alessio alessio deleted the alessio/store-keys-by-hexbytes-repr branch March 30, 2020 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@golangcibot golangcibot golangcibot left review comments

@jgimeno jgimeno jgimeno approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@fedekunze fedekunze Awaiting requested review from fedekunze

@hschoenburg hschoenburg Awaiting requested review from hschoenburg

@nylira nylira Awaiting requested review from nylira

@tac0turtle tac0turtle Awaiting requested review from tac0turtle

Assignees
No one assigned
Labels
C:Crypto C:Keys Keybase, KMS and HSMs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alessio @jgimeno @alexanderbez @golangcibot